Skip to content

Harden redis metrics collection against transient errors - #5131

Open
nikola-jokic wants to merge 1 commit into
masterfrom
nikola-jokic/redis-errors-fix
Open

Harden redis metrics collection against transient errors#5131
nikola-jokic wants to merge 1 commit into
masterfrom
nikola-jokic/redis-errors-fix

Conversation

@nikola-jokic

Copy link
Copy Markdown
Contributor

What type of PR is this?

Fix

What this PR does / why we need it

  • Skip streams that vanish between SCAN and XINFO ("no such key") instead of aborting the whole collection cycle
  • Retry scan attempts on transient errors (timeouts, connection failures) with exponential backoff, bounded by MaxRetries, and a per-attempt CollectionTimeout
  • Keep the last good metrics snapshot on failed cycles; stale metrics are preferable to missing metrics

@datadog-armadaproject

datadog-armadaproject Bot commented Aug 25, 2026

Copy link
Copy Markdown

Pipelines

⚠️ Warnings

⚠️ Your PR has warnings. Please review the issues below.

🚦 2 Pipeline jobs failed

CI | All jobs succeeded

View in Datadog · View in GitHub Actions

CI | test / Golang Unit Tests

View in Datadog · View in GitHub Actions

1 failed test. Error: Should be empty, but was [0xa4f02a64200 0xa4f02a64300 0xa4f02a64400 0xa4f02a64500 0xa4f02a948a0 0xa4f02a46080 0xa4f02a460c0] in collector_test.go:337.

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: db16182 | Docs | View more details | Give us feedback!

@greptile-apps

greptile-apps Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR hardens Redis metrics collection against transient failures while retaining the last successful business-metrics snapshot.

  • Retries transient scan failures with bounded exponential backoff and per-attempt timeouts.
  • Publishes failure telemetry without clearing the last successful Redis metrics.
  • Skips streams that disappear between discovery and inspection.
  • Validates retry backoff settings and safely formats wrapped or ordinary configuration errors.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
internal/eventingester/metrics/redis/collector.go Adds bounded scan retries and republishes updated failure telemetry while retaining the last successful business metrics.
internal/eventingester/configuration/validation.go Validates effective retry-backoff bounds, including interactions between configured maxima and default initial values.
internal/common/config/validation.go Safely formats nil, wrapped validator, and ordinary errors without an unchecked type assertion.
internal/eventingester/repository/scanner.go Treats vanished Redis streams as expected scan races instead of aborting collection.
internal/eventingester/metrics/redis/collector_test.go Covers stale-snapshot retention, failure telemetry, retry classification, retry exhaustion, and effective backoff capping.

Sequence Diagram

sequenceDiagram
    participant C as Metrics collector
    participant R as Redis scanner
    participant S as Atomic snapshot
    C->>R: ScanAll with per-attempt timeout
    alt Scan succeeds
        R-->>C: Stream information
        C->>C: Reset and rebuild business metrics
        C->>S: Publish successful snapshot
    else Retryable failure
        R-->>C: Timeout or connection error
        C->>C: Wait with bounded exponential backoff
        C->>R: Retry ScanAll
    else Final or non-retryable failure
        R-->>C: Error
        C->>C: Increment failure telemetry
        C->>S: Republish retained business metrics plus current error telemetry
    end
Loading

Reviews (11): Last reviewed commit: "Fix redis collection" | Re-trigger Greptile

Comment thread internal/eventingester/metrics/redis/collector.go
Comment thread internal/eventingester/metrics/redis/collector.go Outdated
Comment thread internal/eventingester/configuration/types.go Outdated
Comment thread internal/eventingester/configuration/validation.go Outdated
Comment thread internal/eventingester/configuration/validation.go Outdated
// Collect snapshot with error metrics
c.collectSnapshot()
c.collectionDuration.WithLabelValues(collectionStatusError).Observe(time.Since(start).Seconds())
c.collectSnapshot() // Update snapshot with self-monitoring metrics even on error

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Cancellation test now fails

When the first collection attempt is cancelled, this call publishes a non-empty snapshot containing errorsTotal and the error-labelled collectionDuration, but TestCollect_ContextCancellation requires the collected metrics to remain empty, causing the test suite to fail.

Knowledge Base Used: Event store and ingestion

@nikola-jokic
nikola-jokic force-pushed the nikola-jokic/redis-errors-fix branch 2 times, most recently from 570cedc to fbfcfdf Compare August 27, 2026 12:20
@nikola-jokic
nikola-jokic force-pushed the nikola-jokic/redis-errors-fix branch from db16182 to d7be53e Compare August 28, 2026 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant